stream: finished on closed OutgoingMessage by ronag · Pull Request #34313 · nodejs/node

ronag · 2020-07-11T21:01:41Z

finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: #34274

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ronag · 2020-07-11T21:01:50Z

@nodejs/streams

nodejs-github-bot · 2020-07-11T21:02:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32285/

nodejs-github-bot · 2020-07-11T21:28:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32286/

nodejs-github-bot · 2020-07-11T22:22:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32288/

nodejs-github-bot · 2020-07-11T23:03:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32290/

mcollina

lgtm

rickyes · 2020-07-12T05:01:29Z

finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: #34274

The fix should be #34301

finished should invoke callback on closed OutgoingMessage the same way as for regular streams. Fixes: nodejs#34301

nodejs-github-bot · 2020-07-12T07:38:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32297/

nodejs-github-bot · 2020-07-13T17:38:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32316/

nodejs-github-bot · 2020-07-13T17:41:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32317/

ronag · 2020-07-14T12:18:52Z

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

Trott · 2020-07-14T18:53:16Z

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

Does it fail for you locally with tools/test.py --worker test/parallel/test-bootstrap-modules.js?

richardlau · 2020-07-14T19:08:03Z

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

Does it fail for you locally with tools/test.py --worker test/parallel/test-bootstrap-modules.js?

👍

This test is designed to highlight when we cause extra modules to be loaded during the bootstrap and make sure that we're are doing that conciously. In this case, we're now dragging in https://github.com/nodejs/node/blob/master/lib/internal/http.js during bootstrap with workers because workers uses streams. This is in turn also dragging in the internal performance binding:

node/lib/internal/http.js

Line 8 in 66810a0

const { PerformanceEntry, notify } = internalBinding('performance');

If loading those extra modules now when we bootstrap worker threads is acceptable, add them to the appropriate parts of the test (there's a conditional section for workers). Otherwise consider lazy loading the extra module(s) so they are only loaded if needed.

ronag · 2020-07-14T22:13:51Z

I'm unsure how to fix this then... where do we put kClosed which is only used for http?

lib/internal/streams/end-of-stream.js

nodejs-github-bot · 2020-07-15T18:55:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32361/

lib/internal/streams/end-of-stream.js

ronag · 2020-07-15T21:40:17Z

@nodejs/http

nodejs-github-bot · 2020-07-15T22:27:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32363/

mcollina

lgtm

ronag · 2020-07-16T06:37:48Z

Landed in a55b77d

finished should invoke callback on closed OutgoingMessage the same way as for regular streams. Fixes: #34301 PR-URL: #34313 Fixes: #34274 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

vweevers · 2020-07-16T06:38:19Z

Could be an argument for reconsidering #33990

ronag · 2020-07-16T06:41:39Z

Could be an argument for reconsidering #33990

I don't think internal difficulties should motivate public APIs?

vweevers · 2020-07-16T07:17:06Z

OutgoingMessage has ._closed, fs streams have .closed, might be worth consolidating. Without it, can finished() detect already-closed userland streams?

ronag · 2020-07-16T07:31:57Z

Without it, can finished() detect already-closed userland streams?

Yes? Using the internal property _state.closed? Depends on what you mean by "userland streams". You mean streams that are streamlike but not actually streams?

OutgoingMessage has ._closed

internal property

fs streams have .closed

public property

I'm not against it (I opened that PR after all) but I'm not for it either. I don't think this specific case motivates it.

finished should invoke callback on closed OutgoingMessage the same way as for regular streams. Fixes: #34301 PR-URL: #34313 Fixes: #34274 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

ronag added stream Issues and PRs related to the stream subsystem. dont-land-on-v10.x labels Jul 11, 2020

ronag requested review from lpinca and mcollina July 11, 2020 21:01

mcollina approved these changes Jul 11, 2020

View reviewed changes

github-actions bot mentioned this pull request Jul 12, 2020

CI Relability 2020-07-12 JaneaSystems/nodejs-reliability#35

Open

12 tasks

lpinca approved these changes Jul 12, 2020

View reviewed changes

stream: finished on closed OutgoingMessage

cd753c4

finished should invoke callback on closed OutgoingMessage the same way as for regular streams. Fixes: nodejs#34301

ronag force-pushed the outgoing-finished branch from 56439f2 to cd753c4 Compare July 12, 2020 07:38

github-actions bot mentioned this pull request Jul 13, 2020

CI Relability 2020-07-13 JaneaSystems/nodejs-reliability#36

Open

9 tasks

github-actions bot mentioned this pull request Jul 14, 2020

CI Relability 2020-07-14 JaneaSystems/nodejs-reliability#37

Open

10 tasks

github-actions bot mentioned this pull request Jul 15, 2020

CI Relability 2020-07-15 JaneaSystems/nodejs-reliability#38

Open

13 tasks

mcollina requested changes Jul 15, 2020

View reviewed changes

lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved

tiagonapoli mentioned this pull request Jul 15, 2020

Add Jaeger Prometheus metrics vtex/node-vtex-api#402

Merged

4 tasks

fixup

49770d1

mcollina reviewed Jul 15, 2020

View reviewed changes

lib/internal/streams/end-of-stream.js Show resolved Hide resolved

fixup

ce093e3

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 15, 2020

github-actions bot mentioned this pull request Jul 16, 2020

CI Relability 2020-07-16 JaneaSystems/nodejs-reliability#39

Open

13 tasks

mcollina approved these changes Jul 16, 2020

View reviewed changes

ronag closed this Jul 16, 2020

github-actions bot mentioned this pull request Jul 17, 2020

CI Relability 2020-07-17 JaneaSystems/nodejs-reliability#40

Open

14 tasks

lpinca mentioned this pull request Jul 17, 2020

http: fixed invalid path with connect method #34408

Closed

2 tasks

Uh oh!

Conversation

ronag commented Jul 11, 2020

Checklist

Uh oh!

ronag commented Jul 11, 2020

Uh oh!

nodejs-github-bot commented Jul 11, 2020

Uh oh!

nodejs-github-bot commented Jul 11, 2020

Uh oh!

nodejs-github-bot commented Jul 11, 2020

Uh oh!

nodejs-github-bot commented Jul 11, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

rickyes commented Jul 12, 2020

Uh oh!

nodejs-github-bot commented Jul 12, 2020

Uh oh!

nodejs-github-bot commented Jul 13, 2020

Uh oh!

nodejs-github-bot commented Jul 13, 2020

Uh oh!

ronag commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 14, 2020

Uh oh!

richardlau commented Jul 14, 2020

Uh oh!

ronag commented Jul 14, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 15, 2020

Uh oh!

Uh oh!

ronag commented Jul 15, 2020

Uh oh!

nodejs-github-bot commented Jul 15, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 16, 2020

Uh oh!

vweevers commented Jul 16, 2020

Uh oh!

ronag commented Jul 16, 2020

Uh oh!

vweevers commented Jul 16, 2020

Uh oh!

ronag commented Jul 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ronag commented Jul 14, 2020 •

edited

Loading